Skip to content

Conversation

@HammadB
Copy link
Collaborator

@HammadB HammadB commented Dec 4, 2025

Description of changes

Summarize the changes made by this PR.

  • Improvements & Bug fixes
    • ...
  • New functionality
    • Adds read level enum to the frontend APIs and propagates it through to query nodes
    • Note js client tests are failing as they require committing generated files, this will occur in the top of the stack where client changes are addressed.

Test plan

Will be handled by e2e tests at top of stack

  • Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Migration plan

IndexAndWal will be the default everywhere

Observability plan

What is the plan to instrument and monitor this change?

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?

@github-actions
Copy link

github-actions bot commented Dec 4, 2025

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

Copy link
Collaborator Author

HammadB commented Dec 4, 2025

@HammadB HammadB changed the title [ENH] add eventual consistency to the frontend [ENH] Add eventual consistency to the frontend Dec 4, 2025
@HammadB HammadB force-pushed the hammad/eventual_consistency_query branch from 542b0a4 to a4bcf3a Compare January 9, 2026 16:12
@HammadB HammadB force-pushed the hammad/eventual_consistency_frontend branch from b46d7a5 to bf3841d Compare January 9, 2026 16:22
@HammadB HammadB marked this pull request as ready for review January 9, 2026 16:54
@HammadB HammadB requested a review from rescrv as a code owner January 9, 2026 16:54
@propel-code-bot
Copy link
Contributor

propel-code-bot bot commented Jan 9, 2026

Add Read-Level Controls for Eventually Consistent Searches

Introduced a ReadLevel enum across server, worker, and client layers to let searches opt into eventual consistency by skipping WAL reads. SearchRequest now carries a read_level flag from REST/gRPC through orchestrators, enabling the worker to bypass log prefetch when INDEX_ONLY is requested. Python and JS clients surface the flag via new read_level options while maintaining defaults, and new API tests validate both INDEX_AND_WAL and INDEX_ONLY paths.

Key Changes

• Added ReadLevel enum definitions and serialization in rust types, with SearchRequestPayload and SearchRequest now requiring a read level.
• Updated worker KnnFilterOrchestrator to branch on read level, skipping WAL fetches for INDEX_ONLY and logging the choice.
• Extended Python Collection.search and JS Collection.search to accept/read read_level, propagating the option to HTTP requests.
• Exposed read-level switching via REST endpoints and HTTP client logic (chromadb/api/fastapi.py, async_fastapi.py, and generated TS types).
• Added integration tests (chromadb/test/api/test_search_api.py and JS Jest tests) to cover default and index-only search behavior.

Affected Areas

chromadb/api REST handlers and client wrappers
rust/types plan and API request structures
rust/worker KNN orchestration
rust/chroma HTTP client implementation
clients/new-js collection/search logic and tests
chromadb/test API integration tests

This summary was automatically generated by @propel-code-bot

@HammadB HammadB requested a review from sanketkedia January 9, 2026 17:51
@HammadB HammadB changed the base branch from hammad/eventual_consistency_query to graphite-base/5946 January 9, 2026 18:15
@HammadB HammadB force-pushed the graphite-base/5946 branch from a4bcf3a to c516e91 Compare January 9, 2026 18:17
@HammadB HammadB force-pushed the hammad/eventual_consistency_frontend branch from bf3841d to b8dae44 Compare January 9, 2026 18:17
@HammadB HammadB changed the base branch from graphite-base/5946 to hammad/eventual_consistency_query January 9, 2026 18:17
/// Specifies the read level for consistency vs performance tradeoffs.
/// Defaults to IndexAndWal (full consistency).
#[serde(default)]
pub read_level: ReadLevel,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thoughts on having this per SearchPayload v/s one level for the entire request?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did consider this. I kind of prefer it to be homogenous. Reason is that if you are making decisions off this data, you want a consistent read across all the queries.

@sanketkedia
Copy link
Contributor

Some tests are failing, lgtm with one question

@HammadB
Copy link
Collaborator Author

HammadB commented Jan 9, 2026

  • Note js client tests are failing as they require committing generated files, this will occur in the top of the stack where client changes are addressed.

@sanketkedia Please see the comment in the PR about the test failure - `

Note js client tests are failing as they require committing generated files, this will occur in the top of the stack where client changes are addressed.

## Description of changes

_Summarize the changes made by this PR._

- Improvements & Bug fixes
  - /
- New functionality
  - Adds read level to the clients.

## Test plan

_How are these changes tested?_
E2E Tests that check searches succeed were added.
- [x] Tests pass locally with `pytest` for python, `yarn test` for js,
`cargo test` for rust

## Migration plan
None required, the server handles both cases fwd/back.

## Observability plan
Client changes don't entail this for now

## Documentation Changes
Will update docs in a seperate PR
@HammadB HammadB changed the base branch from hammad/eventual_consistency_query to main January 9, 2026 21:12
match self.read_level {
ReadLevel::IndexOnly => {
// For IndexOnly queries, skip log fetching and use empty logs
tracing::info!("Skipping log fetch for IndexOnly read level");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommended

[Performance] Using tracing::info! here will generate a log line for every search query executed with IndexOnly. In a high-throughput scenario (which is the primary use case for IndexOnly), this will flood the logs.

Consider using tracing::debug! instead.

Context for Agents
Using `tracing::info!` here will generate a log line for every search query executed with `IndexOnly`. In a high-throughput scenario (which is the primary use case for `IndexOnly`), this will flood the logs.

Consider using `tracing::debug!` instead.

File: rust/worker/src/execution/orchestration/knn_filter.rs
Line: 302


# Just verify the API works and returns a valid response structure
assert results["ids"] is not None
assert len(results["ids"]) == 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

[Logic] The assertion len(results["ids"]) == 1 contradicts the comment below it: "Results may be empty if data hasn't been compacted yet".

Since INDEX_ONLY skips the WAL and you just performed an add() (which writes to the WAL), the data might not be in the compacted segments yet, leading to 0 results. Asserting exactly 1 result makes this test flaky or incorrect for eventual consistency testing.

Consider asserting len(results["ids"]) >= 0 or ensuring compaction runs before this assertion if you strictly require the data to be visible.

Context for Agents
The assertion `len(results["ids"]) == 1` contradicts the comment below it: "Results may be empty if data hasn't been compacted yet". 

Since `INDEX_ONLY` skips the WAL and you just performed an `add()` (which writes to the WAL), the data might not be in the compacted segments yet, leading to 0 results. Asserting exactly 1 result makes this test flaky or incorrect for eventual consistency testing.

Consider asserting `len(results["ids"]) >= 0` or ensuring compaction runs before this assertion if you strictly require the data to be visible.

File: chromadb/test/api/test_search_api.py
Line: 76

@HammadB HammadB enabled auto-merge (squash) January 9, 2026 21:18
@HammadB HammadB merged commit 8d71ef1 into main Jan 12, 2026
66 checks passed
chroma-droid pushed a commit that referenced this pull request Jan 12, 2026
## Description of changes

_Summarize the changes made by this PR._

- Improvements & Bug fixes
  - ...
- New functionality
- Adds read level enum to the frontend APIs and propagates it through to
query nodes
- Note js client tests are failing as they require committing generated
files, this will occur in the top of the stack where client changes are
addressed.

## Test plan
Will be handled by e2e tests at top of stack
- [x] Tests pass locally with `pytest` for python, `yarn test` for js,
`cargo test` for rust

## Migration plan
IndexAndWal will be the default everywhere

## Observability plan

_What is the plan to instrument and monitor this change?_

## Documentation Changes

_Are all docstrings for user-facing APIs updated if required? Do we need
to make documentation changes in the [docs
section](https://github.com/chroma-core/chroma/tree/main/docs/docs.trychroma.com)?_
tanujnay112 pushed a commit that referenced this pull request Jan 12, 2026
This PR cherry-picks the commit 8d71ef1
onto rc/2026-01-09. If there are unresolved conflicts, please resolve
them manually.

Co-authored-by: Hammad Bashir <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants